Skip to content

build: migrate prettier config to typescript and remove eslint-plugin-prettier #540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 8, 2025

Conversation

michaelfaith
Copy link
Contributor

This change moves from using eslint-prettier-plugin to using eslint-config-prettier and prettier directly for format checking.

A result of us never running prettier on the root files, is that there were some adjustments needed. I ran prettier on the repo and have added those changes too.

Lastly, I moved the config to typescript.

@michaelfaith michaelfaith force-pushed the build/move-to-prettier-ts branch from d107e8e to a84f50c Compare August 4, 2025 23:15
@michaelfaith
Copy link
Contributor Author

This is failing because the setup-node action is still pulling 22.17.1, instead of 22.18.0, which doesn't have the type stripping.

image

I'm going to add check-latest: true to that steps action, to force it to check for the latest node version

@michaelfaith
Copy link
Contributor Author

That worked

image

But there's still an issue with one of the lint commands. Looking into it

@michaelfaith
Copy link
Contributor Author

michaelfaith commented Aug 5, 2025

What I'm finding is that when we run on a version of node that doesn't include native type stripping (22.17.1), the eslint docs config that uses the prettier api directly can't load the prettier.config.ts, since prettier needs type stripping. But when I force the step to use 22.18.0, eslint is erroring out saying that no file patterns matched the config.

Current theory: with native type stripping in node, eslint doesn't try to use jiti to load the config, but eslint won't use node's native type stripping without the "unstable" flag eslint --flag unstable_native_nodejs_ts_config. When I add that flag, eslint has trouble loading our plugin from source, because all of our extensions are .js instead of .ts.

So, what I'm thinking I'll do, is put a separate PR to change all of the relative import paths to use .ts extensions, and add the rewriteRelativeImportExtensions, that was added in TS 5.7 to allow .ts extensions on relative paths. That'll unblock us from using the latest node with unflagged typestripping.

What's especially interesting, is I wasn't able to reproduce the error locally (hence all of the test commits lol)

Edit: I actually think it's more to do with #542 now. But it'll still be good to do the other change, to support native node type stripping.

…-prettier

This change moves from using `eslint-prettier-plugin` to using `eslint-config-prettier` and prettier directly for format checking.

A result of us never running prettier on the root files, is that there were some adjustments needed.  I ran prettier on the repo and have added those changes too.

Lastly, I moved the config to `typescript`.
@michaelfaith michaelfaith force-pushed the build/move-to-prettier-ts branch from 3fe4a5d to 262946e Compare August 8, 2025 01:21
@michaelfaith michaelfaith marked this pull request as ready for review August 8, 2025 08:45
@michaelfaith michaelfaith requested a review from bmish August 8, 2025 08:45
'plugin:unicorn/recommended',
),
pluginN.configs['flat/recommended'],
prettier,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we technically want to include this last to ensure it overrides any incompatible config?

eslint.config.js (flat config): Import eslint-config-prettier, and put it in the configuration array – after other configs that you want to override.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will generally put the prettier config after all of the reusable configs, but before any configs that I've created with overrides / additional rules. The idea being that anything I add myself is an intentional choice and should take precedence, which I think aligns with the spirit of that documentation. With that said, it's currently before the TS Configs, so I should at least move it to be after those (I authored this PR before adding the TS configs). Would you prefer I move it all the way to the end or just after the ts configs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your reasoning makes sense. I'll leave it up to you.

"lint": "npm-run-all --continue-on-error --aggregate-output --parallel lint:*",
"lint:docs": "markdownlint \"**/*.md\"",
"lint:eslint-docs": "npm-run-all -s build \"update:eslint-docs -- --check\"",
"lint:js": "eslint --cache --ignore-pattern \"**/*.md\" .",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the path needed?

Copy link
Contributor Author

@michaelfaith michaelfaith Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's optional. It defaults to current working directory. I can add it back, if you prefer?

https://eslint.org/docs/latest/use/command-line-interface
Image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine to use default then.

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bmish bmish merged commit 575cb27 into eslint-community:main Aug 8, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants